make leading underscores in ingest consistent #341
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson. Looks like this change has caused a number of tests to fail. In some cases, this may be a sign that we need to change the tests; in other cases, it may be revealing some bad assumptions in other code that are exposed by normalizing the directory path here. Next time we get together, I should give you a tour of the test suite so you can work through fixing these things!
|
|
||
| constructor(dir: string, config: Config, queue: QueueManager) { | ||
| this.dir = dir; | ||
| this.dir = dir.endsWith("/") ? dir : dir + "/"; |
There was a problem hiding this comment.
It also occurred to me that if we wanted to trim the trailing slash rather than normalize it on, we could use a regular expression, like:
this.dir = dir.replace(/\/+$/, "");
... while I still think I like having the leading underscore, I do wonder if it's more technically correct not to have it. Maybe the right thing to do would be to fix the bug by stripping the trailing slash, and then add a feature where we can define a "job name prefix" value in the config file. That might be best of both worlds. I also suggest this in part because fewer tests may fail if we go in the opposite direction.
Anyway, just giving options for you to think about. We can discuss further!
2 participants
TODO